Skip to content

Scale Table.INSET by zoom level instead of using fixed pixels #2456

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Sep 22, 2025
Merged

Scale Table.INSET by zoom level instead of using fixed pixels #2456

merged 1 commit into from
Sep 22, 2025

Conversation

@ShahzaibIbrahim
Copy link
Contributor

@ShahzaibIbrahim ShahzaibIbrahim commented Aug 27, 2025

The Table.INSET constant defines the spacing in the width of a table cell. Previously, this was a fixed value of 3px, which looks correct at 100% zoom but causes the image and text to appear cramped on higher-DPI displays.

This change defines INSET in points and converts it to pixels based on the current zoom level (e.g. 6px at 200%), ensuring consistent spacing across different monitor scale.

How to Test

  • Run the following snippet with 100% primary and 250% secondary monitor zoom
  • Note down the width of a cell by moving it from one monitor to other.
  • See if the values adhere to zoom levels.
import org.eclipse.swt.*;
import org.eclipse.swt.graphics.*;
import org.eclipse.swt.widgets.*;

public class TableInsetTest {

public static void main (String [] args) {
	System.setProperty("swt.autoScale", "quarter");
	System.setProperty("swt.autoScale.updateOnRuntime", "true");
	Display display = new Display ();
	Shell shell = new Shell (display);
	shell.setText("Table Inset Test");
	Table table = new Table (shell, SWT.BORDER | SWT.MULTI);
	table.setLinesVisible(true);
	Rectangle clientArea = shell.getClientArea ();
	table.setBounds (clientArea.x, clientArea.y, 200, 200);
	for (int i=0; i<128; i++) {
		TableItem item = new TableItem (table, SWT.NONE);
		item.setText ("Item " + i);
	}
	table.setSelection (95);
	shell.pack ();
	shell.open ();
	while (!shell.isDisposed ()) {
		if (!display.readAndDispatch ()) display.sleep ();
	}
	display.dispose ();
}
}

Results:
Before Change
Width 150% = 77
Width 250% = 128

After change
Width 150% = 84
Width 250% = 139

@ShahzaibIbrahim ShahzaibIbrahim linked an issue Aug 27, 2025 that may be closed by this pull request
Scale Table.INSET by zoom level instead of using fixed pixels vi-eclipse/Eclipse-Platform#424
Closed
@github-actions
Copy link
Contributor

github-actions bot commented Aug 27, 2025 •
edited
Loading

Test Results

   546 files  ±0     546 suites  ±0   32m 59s ⏱️ +43s
 4 431 tests ±0   4 410 ✅  - 4   17 💤 ±0  4 ❌ +4 
16 764 runs  ±0  16 633 ✅  - 4  127 💤 ±0  4 ❌ +4 

For more details on these failures, see this check.

Results for commit feb86f6. ± Comparison against base commit 508ad4a.

♻️ This comment has been updated with latest results.

@akoch-yatta akoch-yatta force-pushed the master-404-TABLE-INSET branch from c5d338a to 0d2c14c Compare September 4, 2025 11:09
@ShahzaibIbrahim
Scale Table.INSET by zoom level instead of using fixed pixels
feb86f6 
The Table.INSET constant defines the spacing in the width of a table
cell. Previously, this was a fixed value of 3px, which looks correct at
100% zoom but causes the image and text to appear cramped on higher-DPI
displays.

This change defines INSET in points and converts it to pixels based on
the current zoom level (e.g. 6px at 200%), ensuring consistent spacing
across different monitor scale.
fedejeanne
fedejeanne approved these changes Sep 22, 2025
View reviewed changes
Copy link
Member

@fedejeanne fedejeanne left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, as did the same changes in Tree before.

One thing I observed during testing (unrelated to this PR!) is that if I move between monitors, the scrollbars are missing on the small monitor.

My setting

  • 175% monitor on the left (primary)
  • 100% monitor on the right

To reproduce
Start the snippet with these VM arguments:

-Dswt.autoScale=quarter
-Dswt.autoScale.updateOnRuntime=true

Move from 175% monitor to 100% monitor.

Screenshots

175% (OK)

image

100% (No scrollbar)

image

Is this known? Can you please create a ticket for it if it isn't?

@fedejeanne
Copy link
Member

fedejeanne commented Sep 22, 2025

Failing test unrelated: #2516

@HeikoKlare
Copy link
Contributor

HeikoKlare commented Sep 22, 2025

One thing I observed during testing (unrelated to this PR!) is that if I move between monitors, the scrollbars are missing on the small monitor.

It's not the scrollbar that is missing, it's the table not adapting its size upon DPI change while the shell does it. If you enlarge the shell, you will see the scrollbar. Note that this is probably the case for every control, not only for Table.
Usually, size adaptation is done by the applied layout, which is why you only experience that issue when not using any layout for arranging controls. Since no one cared so far, I am not sure if there is really someone caring about a fix for it.

@laeubi
Copy link
Contributor

laeubi commented Sep 22, 2025

@HeikoKlare I do not agree that a Layout must be used... as mentioned elsewhere sizing controls directly is absolutely valid for SWT (not that I would recommend it...)!

So one would say, if no Layout is installed, the shell would probably like to scale up/down its childs, otherwise ask the layout to relayout.

@HeikoKlare
Copy link
Contributor

HeikoKlare commented Sep 22, 2025

I do not agree that a Layout must be used... as mentioned elsewhere sizing controls directly is absolutely valid for SWT (not that I would recommend it...)!

I do not find the place where I said that you must use a layout. In any case, that's not what I wanted to say. I just explained the reason for the behavior in case of this snippet. And I explained why the issue was probably not identified so far. It's of course fully valid, but given the number of users we know about that currently require non-layouted controls to adapt to DPI changes (which to my knowledge is 0), it's probably not worth to invest effort into that right now.

@fedejeanne
Copy link
Member

fedejeanne commented Sep 22, 2025

Since the issue is unrelated, I'm merging this PR.

@fedejeanne fedejeanne merged commit 35119f0 into eclipse-platform:master Sep 22, 2025
15 of 17 checks passed
@fedejeanne fedejeanne deleted the master-404-TABLE-INSET branch September 22, 2025 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fedejeanne fedejeanne fedejeanne approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Scale Table.INSET by zoom level instead of using fixed pixels

4 participants

@ShahzaibIbrahim @fedejeanne @HeikoKlare @laeubi